Load test classes with runtime classloader #34681

holly-cummins · 2023-07-11T14:12:56Z

Bugs fixed by this PR

Resolves QuarkusTest: consider removing the test profile support for @Nested tests #45349
Resolves Broken QuarkusTestExtension #8446
Resolves Kotlin junit ParameterizedTest with functions as argument, not working anymore (Kotlin 2.0) #42000
Resolves Regression in 3.13.0.CR1: Lambda expression from custom serializable interface used as @QuarkusTest method parameter fails with ClassNotFoundException #42006
Resolves @ParameterizedTest with @MethodSource do not work when using @QuarkusTest annotation #44320
Resolves Mockito fails to mock non-public inner class in continuous testing due to classloading issues #38987
Resolves ConfigProvider.getConfig().getOptionalValue("..").get() does not work in JUnitExtension any more #46383
Resolves Ecosystem CI failure NoClassDefFoundError: io/quarkiverse/pact/devmodetest/farm/FarmContractTest quarkiverse/quarkus-pact#272

Bugs created by this PR (doh!)

Outstanding issues/breaking changes (input to release notes)

@TestProfile on @Nested tests are not supported anymore (fixes QuarkusTest: consider removing the test profile support for @Nested tests #45349).
Changes the order of some tests; tests should not depend on execution order if they don't set one explicitly, so this shouldn't cause customer failures, but it did cause failures for us (see, for example, Tests in opentelemetry-reactive project are sensitive to execution order #45955 and Improve MetricsTest reliability #45960)
Dev services start in the discovery phase (Dev Services should be started during startup, not augmentation #45785, we know how to fix this quickly)
Maven surefire.rerunFailingTestsCount option does not work in the case where there are test profiles or resources, unless the test order falls such that the failing test uses the last profile/resource (Maven surefire.rerunFailingTestsCount option does not work in the case where there are test profiles or resources, unless the test order falls such that the failing test uses the last profile/resource #46048, harder to fix)
Increased memory footprint running tests. For suites using multiple profiles and resources, more metaspace may be needed.

What problem is this solving?

We see a lot of problems caused by the fact that we load test classes with the deployment classloader, and then intercept the execution and reload the classes with the runtime classloader. Although the new test is loaded with the runtime classloader, its arguments are still loaded with the system classloader. To work around that we sometimes need to clone the arguments by serializing and de-serializing. This was always brittle and no longer worked at all on Java 17+ (until #40601 fixed that). We also see issues because parts of the test infrastructure see the 'wrong' instance of the class. See, for example, quarkiverse/quarkus-pact#73 and #22611.

We have several feature raised against the JUnit team to allow us more control over classloading. The first of this features was introduced in JUnit 5.10, and allows an interceptor to be registered before any tests are launched. This interceptor can set a thread context classloader, which is then used by JUnit to load tests.

My experiments with this feature were thoroughly disappointing. It turns out, setting a TCCL early in the test lifecycle doesn't really help us, because we overwrite our 'early' TCCL with other TCCLs later in the test lifecycle. The following diagram shows some of the places we set the TCCL.

Source: https://excalidraw.com/#json=HFPHIKx8wv0iiyXgNhAzw,8IlEmPcMRvm9pfCGShdClQ

What if we just used one of the existing interception points to set the 'right' classloader, before tests are loaded? If the tests were loaded with our preferred classloader, we wouldn’t need to intercept the factory. Loading the tests with the runtime classloader needs us to move some of our app initialisation earlier in the lifecycle, but I don't think there's any fundamental barrier to this. (We would have had to do this with a solution based on the new JUnit Launcher Interceptor anyway.)

The logic for starting Quarkus needs to be in the test discovery phase, rather than in the extension. This allows us to create the runtime classloader before the test is loaded. The JUnitTest runner already knows about the Quarkus Extension, so it’s only a small extra bit of knowledge to do some of the startup actions.

This only gets us part of the way, though. @stuartwdouglas raised the point that if we have to set only a single classloader, that's not very flexible, because we have a runtime classloader for each test profile. A Quarkus test run doesn't just use one classloader, it uses several. Every resource/unique profile triggers an app relaunch, which means a new classloader. What I've done to handle this is create a FacadeClassLoader. It takes the classloading requests, and then either routes them on to the quarkus application (for vanilla @QuarkusTests), or, if there's a profile/resource, it makes a new app + classloader and sends the request to that.

What we used to before was load a throwaway copy of the the test, pass it to JUnit discovery, let JUnit launch it, and then intercept the execution, figure out what profiles+resources the test declares, create a quarkus app with that information, start the quarkus app, reload the test with the runtime classloader of the quarkus app (and clone its parameters), and execute the test.

The new model is load a throwaway copy of the the test, figure out what profiles+resources the test declares, create a quarkus app with that information, reload the test with the runtime classloader of the quarkus app, pass the ‘right’ class to JUnit discovery, let JUnit launch it, and then intercept the execution, start the quarkus app, and execute the test.

One fundamental limitation of "load tests with the classloader used to execute them" is that a single test cannot run with multiple classloaders, which means it cannot support multiple profiles. We know some people do use this feature, but we also know there have been suggestions that we drop support for it, since it is complex to support (#45349). There is an easy workaround, which is to use one test per profile.

Thoughts on serialization and cloning

A big initial goal of this PR was to get rid of the xstream serialization, since it didn't work on Java 17+. #40601 fixes this issue by switching to use the JBoss marshaller for serialization. Does that mean this work item isn't needed any more? No, although it does mean its benefits are smaller. Here's why it's still useful:

Even with the JBoss serializer, higher-level test infrastructure (such as @TestTemplate) does not see Quarkus bytecode transformations done by extensions
Although the JBoss serializer works a lot better than xstream with the Java 17 access restrictions (as in, it works), serialization may continue to be a challenge going forward. See https://bugs.openjdk.org/browse/JDK-8164908 for some context. Most serializers use sun.misc.unsafe, but unsafe is shrinking. It seems certain the JDK team will have to come up with some solution and API to open up access for serializers, but the final design could have security implications (perhaps opening up access in a blanket way), or performance implications (reflection fun), or user experience implications (a need to manually set flags such as --enable-serialization?). If we can avoid serialization, we avoid all that.

Todo before this merges

Start investigating parallel tests
Start looking at classloader leaks
Find Holly's flaky test reproducer that she lost somewhere in a branch
Fix the ecosystem CI for pact

Todo after this merges

Monitor ecosystem CI for new failures; we do not have coverage of every code path in our current suite (for example, the tests added in Continuous Testing: add support for build system like test selection #46389 broke with this change, by going down an uncovered code path)
Address the 'do not start dev services in augmentation' issue
Removal of all dead code in QuarkusTestExtension
Consolidation of duplicated code between AppMakerHelper and TestSupport
More automated tests (will come in a PR that goes in first)
Tests for interaction with QuarkusProdModeTest, particularly for the tests in Add tests which exercise more complex JUnit extensions #35124
Continued improvements to fix hacky config usage in existing tests (such as relying on mutable system properties, etc)
Consolidation + streamlining of logic in the test order (unify around a key-based approach?)
Adding tests based on various outstanding issues which do not yet have reproducers in our test suite

The-Funk · 2023-09-14T18:15:32Z

Still watching this one. I've created a minimal not-working-example to see if this fixes it. :)

holly-cummins · 2023-09-15T19:32:55Z

Still watching this one. I've created a minimal not-working-example to see if this fixes it. :)

Still going on it ... :)

The profile support in normal mode turned out to be a bit thorny, so looking at that now. I haven't checked for a while, but at one point I had reproducers for three of the test-classloading-related defects. One was passing, but two (annoyingly) were failing. If your reproducer is shareable, I'm happy to take it and include it in what I'm checking, or to add it into the test suites, if it's a gap in what we're testing now. (It'd be worth checking what I've added in #35124 to see if one of those covers your scenario, too.)

geoand · 2025-03-11T09:01:53Z

...pendent-projects/bootstrap/core/src/main/java/io/quarkus/bootstrap/app/QuarkusBootstrap.java

@@ -309,12 +311,21 @@ public Supplier<DependencyInfoProvider> getDependencyInfoProvider() {
        return depInfoProvider;
    }

+    // TODO where is the cleanest place for this to live? We don't want code which has been given classes


cc @aloubyansky

I think ApplicationModel.getAppArtifact().getWorkspaceModule().getTestSources() and then either getOutputTree() or getSourceDirs().getOutputDir() is meant to be that. But we need to see how/where it fits in this case.

geoand · 2025-03-11T09:06:19Z

...ndent-projects/bootstrap/core/src/main/java/io/quarkus/bootstrap/app/CuratedApplication.java

@@ -440,6 +448,11 @@ public void close() {
        augmentationElements.clear();
    }

+    // TODO delete this? the model doesn't really work?


Doesn't really work, in what sense?

Good question. That comment is so old I wasn't entirely sure myself, when I was reviewing the TODOs. :)

I think what it's referring to is that at an early stage of development I had good ideas about re-using more of the curated application + base classloaders between different applications. That is, if I decided we needed to restart, instead of throwing everything out, I'd do a light tidy and then re-use the lower levels of the classloader stack. That might still be a good thing to do, and it would reduce the memory footprint of the new 'load everything upfront' pattern. If we can do restarts for continuous testing we should be able to achieve some reuse in normal mode testing. But I never made it work.

So I think that tidy() method is a vestige of my attempts to re-use stuff. The question is whether it's now totally pointless because cleanup happens elsewhere, or whether it's still a useful part of the cleanup that we do between restarts. I'll inspect the code and try and work it out.

holly-cummins · 2025-03-11T09:07:35Z

@geoand , FYI I've just spotted that these changes would break the JBeret Ecosystem CI. I think I know what the fix is, but I'll need to make an update. Hopefully it won't be a big update, just a tweak to the quarkus test detection logic to catch whatever edge case is breaking it in the JBeret tests.

geoand · 2025-03-11T09:10:46Z

Thanks for the heads up!

geoand · 2025-03-11T09:26:11Z

test-framework/junit5/src/main/java/io/quarkus/test/junit/classloading/FacadeClassLoader.java

+ * would allow us to swap the thread context classloader.
+ * Since we can't intercept with a JUnit hook, we hijack from inside the classloader.
+ * <p>
+ * We need to load all our test classes in one go, during the discovery phase, before we start the applications.


We need to load all our test classes in one go, during the discovery phase

Is this a current JUnit limitation?

It is, to the best of my knowledge. I banged my head against it for a while, but I don't see a solution that's not writing a new engine, or doing a pre-test-reload. Pre-test-reload sounds appealing, but it's basically the existing approach. It means JUnit doesn't see our modified test code, because if we reload post-discovery, it's too late for some JUnit functionality.

Got it thanks! Any pointers at where I can place debug breakpoints to see this in action?
I would like to understand the current limitations so after we get this in, we can go back to the JUnit 5 people with a details on what we use so far and why it's not enough.

aloubyansky · 2025-03-12T07:53:10Z

core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java

-                    TestSupport.instance().get()::isDisplayTestOutput);
+                    TestSupport.instance()
+                            .get()::isDisplayTestOutput);
+            // TODO do we want to do this setting of the TCCL? I think it just makes problems?


Any specific hints about the possible problems?

aloubyansky · 2025-03-12T08:35:25Z

test-framework/common/src/main/java/io/quarkus/test/common/PathTestHelper.java

+        } else if (resource.getProtocol().equals("quarkus")) {
+            // This is loaded with a quarkus classloader, so we can ask it directly
+            QuarkusClassLoader qcl = (QuarkusClassLoader) testClass.getClassLoader();
+            return qcl.getCuratedApplication().getQuarkusBootstrap().getTestClassesLocation();


This is where we (theoretically) could try qcl.getCuratedApplication().getApplicationModel().getAppArtifact().getWorkspaceModule().getTestSources().getOutputDir()

aloubyansky · 2025-03-12T08:48:56Z

test-framework/junit5/src/main/java/io/quarkus/test/junit/AppMakerHelper.java

+        if (curatedApplication == null) {
+            curatedApplication = makeCuratedApplication(requiredTestClass, displayName, isContinuousTesting, shutdownTasks);
+        }
+        Path testClassLocation = getTestClassLocationIncludingPossibilityOfGradleModel(requiredTestClass);


It seems like testClassLocation should already be available from the curatedApplication here? Could they be different here? If they are, isn't it an issue?

quarkus-bot · 2025-03-12T11:50:37Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 053442c.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/hibernate-orm/deployment

✖ io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.testFieldAccess - History

Expecting actual not to be null - java.lang.AssertionError

java.lang.AssertionError: 

Expecting actual not to be null
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest$FieldAccessEnhancedDelegate$1.assertValue(PublicFieldAccessInheritanceTest.java:141)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.doTestFieldAccess(PublicFieldAccessInheritanceTest.java:100)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.testFieldAccess(PublicFieldAccessInheritanceTest.java:61)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:521)

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario1 - History

Failed to wait for test run 2 State{lastRun=1, running=true, inProgress=true, run=1, passed=0, failed=1, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true} - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Failed to wait for test run 2 State{lastRun=1, running=true, inProgress=true, run=1, passed=0, failed=1, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true}
	at io.quarkus.test.ContinuousTestingTestUtils.waitForNextCompletion(ContinuousTestingTestUtils.java:44)
	at io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario1(KafkaDevServicesContinuousTestingTestCase.java:51)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: org.awaitility.core.ConditionTimeoutException: Condition returned by method "waitForNextCompletion" in class io.quarkus.test.ContinuousTestingTestUtils was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(Conditio...

holly-cummins marked this pull request as draft July 11, 2023 14:13

quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform area/testing labels Jul 11, 2023

holly-cummins mentioned this pull request Jul 12, 2023

Running a QuarkusTest fails due to class load errors with Kotlin after quarkus 3 migration #34099

Open

holly-cummins force-pushed the upgrade-junit branch from f5a756d to f661328 Compare July 21, 2023 14:31

quarkus-bot bot added the area/core label Jul 21, 2023

holly-cummins mentioned this pull request Jul 21, 2023

Quarkus 3 - consumer tests cannot directly access pact MockServer in test methods in dev mode quarkiverse/quarkus-pact#73

Closed

This was referenced Jul 31, 2023

Adding @QuarkusTest support for PactVerifyProvider pact-foundation/pact-jvm#1506

Open

Add tests which exercise more complex JUnit extensions #35124

Merged

holly-cummins force-pushed the upgrade-junit branch from 2c4ddb4 to 8f88f8a Compare August 30, 2023 18:53

quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Aug 30, 2023

holly-cummins marked this pull request as ready for review August 30, 2023 18:56

holly-cummins changed the title ~~Load test classes with runtime classloader~~ Load test classes with runtime classloader (draft) Aug 30, 2023

This comment has been minimized.

Sign in to view

holly-cummins force-pushed the upgrade-junit branch from aba5d5c to 68e6194 Compare August 31, 2023 16:54

quarkus-bot bot added the area/gradle Gradle label Aug 31, 2023

holly-cummins changed the title ~~Load test classes with runtime classloader (draft)~~ Load test classes with runtime classloader Aug 31, 2023

holly-cummins marked this pull request as draft August 31, 2023 21:03

This comment has been minimized.

Sign in to view

quarkus-bot bot added the area/maven label Sep 4, 2023

holly-cummins mentioned this pull request Sep 14, 2023

Bump org.junit:junit-bom from 5.9.3 to 5.10.0 #35518

Merged

holly-cummins mentioned this pull request Nov 8, 2023

Ability to use UniAsserter with @ParameterizedTest #24590

Open

holly-cummins force-pushed the upgrade-junit branch from 4307712 to 181a730 Compare March 22, 2024 18:30

holly-cummins marked this pull request as ready for review March 22, 2024 18:31

holly-cummins changed the title ~~Load test classes with runtime classloader~~ Draft: Load test classes with runtime classloader Mar 22, 2024

holly-cummins mentioned this pull request Mar 5, 2025

Improve type safety of method signature #46625

Merged

This comment has been minimized.

Sign in to view

holly-cummins force-pushed the upgrade-junit branch from d69004e to 9db70c2 Compare March 5, 2025 14:55

This comment has been minimized.

Sign in to view

holly-cummins force-pushed the upgrade-junit branch from 8a702df to 7a74a0d Compare March 6, 2025 08:01

This comment has been minimized.

Sign in to view

holly-cummins force-pushed the upgrade-junit branch from 7a74a0d to ceed7b9 Compare March 6, 2025 14:06

This comment has been minimized.

Sign in to view

holly-cummins force-pushed the upgrade-junit branch from 5d9fac9 to 1bdd946 Compare March 7, 2025 12:40

holly-cummins mentioned this pull request Mar 7, 2025

Chore: Restructure tests for tests #46667

Open

This comment has been minimized.

Sign in to view

holly-cummins force-pushed the upgrade-junit branch from 1bdd946 to 7aa5165 Compare March 10, 2025 10:23

holly-cummins mentioned this pull request Mar 10, 2025

Upgrading from 3.18.2 to 3.18.3 Results in OutOfMemoryError when using @QuarkusTest with Quarkus Junit 5 #46459

Closed

This comment has been minimized.

Sign in to view

geoand changed the title ~~Draft: Load test classes with runtime classloader~~ Load test classes with runtime classloader Mar 11, 2025

geoand reviewed Mar 11, 2025

View reviewed changes

holly-cummins force-pushed the upgrade-junit branch from 7aa5165 to 32496bf Compare March 11, 2025 09:20

geoand reviewed Mar 11, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

aloubyansky reviewed Mar 12, 2025

View reviewed changes

holly-cummins added 2 commits March 12, 2025 08:16

Load tests with runtime classloader, including profile support

c5270ee

Tidy println

053442c

holly-cummins force-pushed the upgrade-junit branch from 32496bf to 053442c Compare March 12, 2025 08:16

aloubyansky reviewed Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load test classes with runtime classloader #34681

Load test classes with runtime classloader #34681

holly-cummins commented Jul 11, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

The-Funk commented Sep 14, 2023

holly-cummins commented Sep 15, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

geoand Mar 11, 2025

aloubyansky Mar 12, 2025

geoand Mar 11, 2025

holly-cummins Mar 11, 2025 •

edited

Loading

geoand Mar 11, 2025

holly-cummins commented Mar 11, 2025

geoand commented Mar 11, 2025

geoand Mar 11, 2025

holly-cummins Mar 11, 2025

geoand Mar 11, 2025

This comment has been minimized.

aloubyansky Mar 12, 2025

aloubyansky Mar 12, 2025

aloubyansky Mar 12, 2025

quarkus-bot bot commented Mar 12, 2025 •

edited by github-actions bot

Loading

Load test classes with runtime classloader #34681

Are you sure you want to change the base?

Load test classes with runtime classloader #34681

Conversation

holly-cummins commented Jul 11, 2023 • edited Loading

Bugs fixed by this PR

Bugs created by this PR (doh!)

Outstanding issues/breaking changes (input to release notes)

What problem is this solving?

Thoughts on serialization and cloning

Todo before this merges

Todo after this merges

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

The-Funk commented Sep 14, 2023

holly-cummins commented Sep 15, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holly-cummins Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holly-cummins commented Mar 11, 2025

geoand commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quarkus-bot bot commented Mar 12, 2025 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/hibernate-orm/deployment

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

holly-cummins commented Jul 11, 2023 •

edited

Loading

holly-cummins Mar 11, 2025 •

edited

Loading

quarkus-bot bot commented Mar 12, 2025 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`